Reject submodule move destinations through intermediate symlinks - #2232

This pull request hardens submodule operations against intermediate symlink traversal and adds regression coverage.

Changes:

• Adds centralized checkout-path symlink validation.
• Adds symlink and move regression tests.
• Documents the security fix as version 3.1.63.

• Files reviewed: 3/3 changed files
• Comments generated: 4
• Review effort level: Lite

Suppressed comments (3)

git/objects/submodule/base.py:1237

• When module=True and the checkout path is symlinked, module_exists() below catches the ValueError from self.abspath and returns False. Removal then skips the checkout deletion but continues deleting the index and configuration, so remove(force=True) partially unregisters the submodule instead of rejecting it. Validate the checkout path before calling module_exists().

        self._validated_name(self.name)
        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1103

• Allowing the final symlink here leaves dangling destinations unchecked because the following osp.exists() test returns false for them. On POSIX, _renames() then calls os.rename, which replaces the dangling link and lets the move update repository metadata, contradicting the new test's required OSError and no-side-effects behavior. Reject a dangling final link before the destination-removal branch.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:168

• Because allow_final_symlink=True is used for destination validation, a dangling final symlink is not seen by osp.exists() or osp.isfile(), so move() reaches _renames() and os.rename() replaces the dangling link on POSIX. This branch therefore does not raise OSError and mutates repository state, causing the new test to fail. If dangling final links must be rejected, check lexists before removal; otherwise adjust the expectation to preserve the existing behavior.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (5)

git/objects/submodule/base.py:258

• This new guard only runs when a caller enters _config_parser. update() starts with self.module() and remote fetching and, for an already initialized checkout at the requested revision, can return without opening .gitmodules, so the gitmodules/update case added in this PR does not raise for the symlink. Add the .gitmodules preflight at the start of update(), before fetching.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

git/objects/submodule/base.py:1239

• These new checks cover only .gitmodules; when module=True, the module_exists() call below invokes self.module(), whose abspath now raises for a symlinked checkout, but module_exists() catches every exception and returns False. remove(force=True) therefore skips checkout deletion and continues deleting the index/configuration, so the checkout/remove case added in this PR is not rejected and can partially apply. Validate the checkout path before calling module_exists().

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1105

• allow_final_symlink=True does not handle the new dangling-link case: osp.exists() is false for a dangling symlink, so the removal block is skipped and _renames() calls os.rename(), which replaces that link on POSIX. The kind == "dangling" assertion at test/test_submodule.py:167-168 will therefore fail and the destination link is changed; reject a lexists-but-not-exists destination before renaming.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:167

• On POSIX this branch does not raise for a dangling leaf link: allow_final_symlink=True lets it through, osp.isfile and osp.exists are false, and _renames uses os.rename, which replaces the dangling link. That contradicts this test and the stated preservation of final-component behavior; treat dangling like empty (or add an explicit lexists rejection if rejection is intended).

        with pytest.raises(OSError if kind == "dangling" else ValueError):

test/test_submodule.py:234

• The update branch does not consult .gitmodules when the checkout is already initialized: it calls self.module(), fetches, and can return at the same commit without reaching _config_parser. A symlinked .gitmodules therefore is not rejected and this pytest.raises case fails; add the shared .gitmodules preflight in update() before fetching.

        if operation == "update":
            sm.update()

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (2)

git/objects/submodule/base.py:1241

• This preflight validates .gitmodules but not the checkout. When the checkout is a symlink, module_exists() calls self.module(), catches the ValueError from self.abspath, and returns False; remove() then continues to delete the index and configuration below instead of rejecting the operation. Validate the checkout path before calling module_exists() whenever module is enabled.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

test/test_submodule.py:234

• The gitmodules/update parameter does not reach a rejecting path for an initialized submodule whose HEAD already matches the index: update() can use self.module(), fetch, and finish without invoking _config_parser. Consequently this pytest.raises block fails for that combination. Add an explicit .gitmodules preflight at the start of update() before fetching so the regression case is implemented rather than only asserted.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (4)

git/objects/submodule/base.py:1241

• When the checkout itself is symlinked, this preflight only checks .gitmodules. The module_exists() call below catches the ValueError raised by self.abspath and returns False, so remove(force=True) skips physical deletion and still deletes the index/config entries. Validate the checkout path whenever module is true before recursive/removal work.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1107

• Because allow_final_symlink=True skips the final component, a dangling destination is false for both osp.isfile() and osp.exists() and falls through to _renames, which replaces the dangling link on POSIX. The new kind == "dangling" branch therefore will not raise as asserted; use osp.lexists() for the destination existence check if dangling leaf links are meant to remain rejected.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:235

• The gitmodules + update combination is not rejected when the module is already initialized: update() calls self.module() and fetches the child repository, but does not read .gitmodules, so _config_parser's symlink check is never reached. This parameterized case will fail; add an explicit .gitmodules preflight to update(), or remove this expectation if update() is intentionally allowed to use a symlinked file.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

test/test_submodule.py:169

• On POSIX this expectation does not hold: allow_final_symlink=True lets a dangling leaf through, osp.exists() is false, and _renames() calls os.rename, which replaces the dangling link and completes the move. The assertion therefore fails and the repository state changes; either reject dangling leaf links before the rename (for example with lexists) or update this case to match the intended final-link compatibility behavior.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (2)

git/objects/submodule/base.py:1107

• Allowing the final component here leaves a dangling destination unhandled: both osp.isfile() above and the osp.exists() check below follow links, so a dangling symlink falls through to _renames(), whose os.rename() replaces that link on POSIX. The new kind == "dangling" test therefore fails instead of raising; detect the destination with osp.lexists() (while retaining the existing empty-leaf replacement behavior).

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:233

• The gitmodules + update case does not reach the new guard: for this already initialized checkout, update() proceeds through self.module() and fetch_remotes() without reading .gitmodules, so _config_parser is never called and no ValueError is raised. This parameterized case will fail; either preflight .gitmodules before fetching or remove/adjust this expectation if matching Git's less restrictive behavior is intentional.

    with pytest.raises(ValueError, match="contains a symbolic link"):

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (3)

git/objects/submodule/base.py:1107

• allow_final_symlink=True still treats a dangling destination as absent: osp.exists() below is false, so _renames() replaces the dangling link and the move succeeds on POSIX. The new test_move_leaf_symlink_compatibility case expects an OSError; use osp.lexists() for the destination check (or explicitly reject dangling final links) before moving.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1247

• This preflight only validates .gitmodules. For a symlinked checkout, module_exists() catches the ValueError raised by self.abspath and returns False, so remove(force=True) skips the checkout deletion and continues deleting the index/configuration. That contradicts the new checkout-symlink case at test/test_submodule.py:241; validate the checkout path before calling module_exists().

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:259

• This guard is only reached when a config parser is opened. An initialized update() loads the module, fetches, and can finish without invoking _config_parser, so a symlinked working-tree .gitmodules is accepted while the new matrix at test/test_submodule.py:233 expects ValueError. Add the same .gitmodules preflight to update() before module access, inside its existing error-handling scope.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

test/test_submodule.py:322

• Patching git.objects.submodule.base.osp does not affect pathlib.Path.resolve(), which uses pathlib's own os.path.realpath. _write_git_file_and_module_config() now calls Path.resolve() directly at base.py:487-488, so both fixture parameters exercise the same resolver and the windows37-realpath case does not model the Python 3.7 Windows behavior it claims to cover. Patch the resolver used by pathlib or make the resolution helper injectable.

git/objects/submodule/base.py:1107

• allow_final_symlink=True lets a dangling destination skip the component check, but the later osp.exists() is also false for a dangling link. On POSIX, _renames() then calls os.rename(source, destination), which replaces that dangling symlink, so the new kind == "dangling" test succeeds instead of raising and the move mutates repository state. Treat the destination as present with lexists() before inspecting or replacing it.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1247

• With a symlinked checkout and module=True, module_exists() below catches the ValueError raised by self.module()/self.abspath and returns False. Forced removal then skips checkout deletion but still deletes the index and configuration, so the added checkout/remove case cannot pass and the rejected operation is not side-effect free. Validate the checkout path before calling module_exists(), or remove this case if that compatibility is intentional.

            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

test/test_submodule.py:235

• This initialized-module update case cannot pass for link_kind == "gitmodules": update() calls self.module() and fetch_remotes() directly and never enters _config_parser, so replacing .gitmodules with a symlink is not observed and sm.update() completes without the expected ValueError. Either add an explicit .gitmodules preflight to update(), or remove this operation from the rejection matrix if Git-compatible initialized updates are intentionally allowed.

            sm.update()

test/test_submodule.py:241

• The checkout/remove case does not take this rejection path. With .gitmodules intact, remove() calls module_exists(), which catches the ValueError raised by self.abspath for the symlinked checkout and returns False; removal then continues into index and configuration deletion. The new pytest.raises at test/test_submodule.py:241 therefore fails and the operation is not side-effect-free. If Git-compatible removal is intentional, remove "remove" from this matrix; otherwise preflight the checkout before module_exists().

            sm.remove(force=True)

test/test_submodule.py:308

• git.Repo(alias) resolves a Path input before storing working_tree_dir (git/util.py:592-601), so this test operates on the real parent directory rather than through the alias symlink. It therefore does not exercise the claimed symlink-above-worktree case or the new root-handling path; preserve the symlink in the repository's working-tree path before running the operations.

    with git.Repo(alias) as parent:
        added = Submodule.add(parent, "new", "new", sm.url)

test/test_submodule.py:220

• sm.rename("nested/module") has already moved the checkout from root / "module" to root / "nested/module". For link_kind == "checkout", this path.rename(target) therefore raises FileNotFoundError before the operation under test runs; point the checkout case at the renamed path.

    path = root / (".gitmodules" if link_kind == "gitmodules" else "module")

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (5)

git/objects/submodule/base.py:259

• _config_parser is not reached by update() when the checkout is already initialized: update() calls self.module(), fetches remotes, and can finish without reading .gitmodules. Consequently the new gitmodules/update case in test_submodule_rejects_checkout_and_gitmodules_symlinks returns successfully instead of raising before side effects. Add an explicit .gitmodules preflight to update(), or remove that expectation if this operation is intentionally allowed.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

git/objects/submodule/base.py:1259

• module_exists() below calls self.module() and swallows the ValueError from self.abspath when the checkout itself is symlinked. With module=True, removal therefore skips deleting the checkout and proceeds to index/configuration deletion; for a normal .gitmodules this succeeds, and for a symlinked .gitmodules it fails only after the index and parent config have changed. Validate the checkout path before module_exists() so removal rejects without partial state changes.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1119

• allow_final_symlink=True skips validation of the destination leaf, but the later osp.exists(module_checkout_abspath) is false for a dangling leaf symlink. _renames() then invokes os.rename and replaces that link on POSIX, so the new kind == "dangling" test succeeds and changes repository state instead of raising. Use osp.lexists() for the destination existence check and reject the dangling case before removing or moving anything.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:169

• On POSIX, the new dangling case does not raise: allow_final_symlink=True lets the destination through, osp.exists() is false for a dangling link, and _renames() calls os.rename, which replaces that link. This test therefore fails and conflicts with the stated preservation of final-component behavior; either assert the successful replacement for this case or add an explicit lexists rejection in move if rejection is intended.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

test/test_submodule.py:235

• An initialized update() follows self.module(), fetches remotes, and can reset the checkout without invoking _config_parser, so a symlinked .gitmodules is not rejected by this branch. The gitmodules + update parameter therefore completes instead of raising and makes this new regression test fail; narrow the case to operations that read configuration, or add an explicit update preflight only if rejecting this Git-compatible behavior is intended.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (5)

git/objects/submodule/base.py:1119

• allow_final_symlink=True lets a dangling destination reach the osp.exists() check below, which returns false for dangling links; _renames() then calls os.rename and replaces the link on POSIX. The new kind == "dangling" case therefore does not raise and the move mutates the index/configuration. Use osp.lexists(module_checkout_abspath) for the destination-presence check so dangling links are rejected before the move.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1259

• module_exists() below catches every exception from self.module(), including the ValueError raised by self.abspath for a symlinked checkout. Consequently remove(force=True) skips the module branch and continues to delete the index/configuration instead of rejecting the operation; the new (checkout, remove) regression case fails and can leave partial state. Validate the checkout path before calling module_exists() when module is true.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:259

• This guard is not reached by an initialized update(): it first opens the module and fetches, and when the module is already at self.binsha it never reads .gitmodules. Consequently the new gitmodules + update parameterization does not raise and the test suite fails; either add an update preflight if rejection is intended, or remove that case if Git-compatible behavior is intended.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

test/test_submodule.py:235

• For an initialized submodule, update() calls self.module() and fetches/resets the child repository without opening the working-tree .gitmodules; therefore the (link_kind="gitmodules", operation="update") combination returns successfully instead of entering this pytest.raises. Either add an explicit preflight if rejecting this case is required, or exclude this pair; as written the new test fails.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

test/test_submodule.py:169

• On POSIX this branch cannot raise for a dangling final link: allow_final_symlink=True permits it, both osp.isfile() and the later osp.exists() check return false, and _renames() replaces the dangling link with os.rename(). The added kind == "dangling" case therefore succeeds and mutates repository state instead of entering this exception block; update the expectation to preserve the existing replacement behavior, or add an explicit dangling-link rejection.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite